Simplify the cache_on_disk_if modifier to just cache_on_disk#154576
Simplify the cache_on_disk_if modifier to just cache_on_disk#154576Zalathar wants to merge 3 commits intorust-lang:mainfrom
cache_on_disk_if modifier to just cache_on_disk#154576Conversation
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Simplify the `cache_on_disk_if` modifier to just `cache_on_disk`
|
Ok, that's good |
|
Zulip topic for context: #t-compiler/query-system > Removing key_fingerprint_style and cache_on_disk_if |
|
timeout |
This comment has been minimized.
This comment has been minimized.
Simplify the `cache_on_disk_if` modifier to just `cache_on_disk`
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (5a1a969): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 1.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -1.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 483.836s -> 485.128s (0.27%) |
| _tcx: TyCtxt<'tcx>, | ||
| _key: rustc_middle::queries::$name::Key<'tcx>, | ||
| ) -> bool { | ||
| if !cfg!($cache_on_disk) { |
There was a problem hiding this comment.
Can you use #[cfg] instead of cfg!?
Alternatively, cfg_select! might make this function nicer.
There was a problem hiding this comment.
The cfg_select! version ends up being a bit visually “messier”, but has the benefit of being very explicit about exhaustively listing all possible cases, which seems like an improvement. 👍
There was a problem hiding this comment.
You can actually just use $cache_on_disk.
This is the only remaining query with a non-trivial `cache_on_disk_if` condition, but previous perf experiments indicate that it can be removed entirely, with little or no measurable effect.
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
@rustbot ready |
|
@bors r+ |
Queries with both `cache_on_disk` and `separate_provide_extern` will only disk-cache values for local keys. Other queries with `cache_on_disk` will disk-cache all values unconditionally.
|
Ah, I remembered that the dev-guide will need updating too. Since this is rollup=never and nowhere near the top of the queue, I'll push a fix. |
|
The rustc-dev-guide subtree was changed. If this PR only touches the dev guide consider submitting a PR directly to rust-lang/rustc-dev-guide otherwise thank you for updating the dev guide with your changes. |
|
This pull request was unapproved. |
|
@bors r+ |
|
⌛ Testing commit 076dc9b with merge ba11b1e... Workflow: https://github.com/rust-lang/rust/actions/runs/23799192968 |
Simplify the `cache_on_disk_if` modifier to just `cache_on_disk` Thanks to #154304, there is now only one remaining query with a non-trivial cache-on-disk condition (`check_liveness`). But prior experiments at #153441 (comment) indicate that `check_liveness` can also be made non-disk-caching with little or no measurable effect. This PR takes advantage of those facts to replace the `cache_on_disk_if` modifier with a simpler `cache_on_disk` modifier: - When combined with `separate_provide_extern`, only values for “local” keys are cached to disk. - For any other query with `cache_on_disk`, values are cached to disk unconditionally. r? nnethercote (or compiler)
View all comments
Thanks to #154304, there is now only one remaining query with a non-trivial cache-on-disk condition (
check_liveness). But prior experiments at #153441 (comment) indicate thatcheck_livenesscan also be made non-disk-caching with little or no measurable effect.This PR takes advantage of those facts to replace the
cache_on_disk_ifmodifier with a simplercache_on_diskmodifier:separate_provide_extern, only values for “local” keys are cached to disk.cache_on_disk, values are cached to disk unconditionally.r? nnethercote (or compiler)